Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H-4069, H-4081, H-4110: Data type conversions in data type editor; entity editor fixes #6598

Merged
merged 8 commits into from
Mar 4, 2025

Conversation

CiaranMn
Copy link
Member

@CiaranMn CiaranMn commented Mar 3, 2025

🌟 What is the purpose of this PR?

This PR adds the MVP for data type conversion display and editing in the data type editor.

In the entity editor it also:

  • Fixes an issue where the form would show changes as having been made when a user changed then reverted a value
  • Exposes when an entity is archived / unarchived in the history tab

To keep simple things simple the following restrictions are in place for now:

  1. Data types can only have one conversion target
    • if the data type is a child of a group where its siblings already have a conversion target, it can only be that
  2. Users cannot select data types as conversion targets which themselves define conversions to another target – they must select something without any conversions defined on it (e.g. a canonical, or something which is not part of any converted group)

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

⚠️ Known issues

H-4161: Still some slightly-off / floating point issues in the FE when values undergo multiple conversion steps

🛡 What tests cover this?

  • None yet

❓ How to test this?

  1. Not possible in preview deployment due to Node API changes.

📹 Demo

Adding a data type under a parent which already has a canonical in the group

frequency.mp4

Selecting an arbitrary conversion target from a data type not in a group

length.mp4

@CiaranMn CiaranMn requested a review from vilkinsons March 3, 2025 22:45
@CiaranMn CiaranMn self-assigned this Mar 3, 2025
@github-actions github-actions bot added area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team type/eng > backend Owned by the @backend team area/apps labels Mar 3, 2025
Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 20.85%. Comparing base (8d98984) to head (784dfa8).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...ocal/hash-isomorphic-utils/src/subgraph-mapping.ts 0.00% 7 Missing ⚠️
...sh-api/src/graphql/resolvers/ontology/data-type.ts 0.00% 2 Missing ⚠️
...hash-api/src/graph/ontology/primitive/data-type.ts 0.00% 1 Missing ⚠️
.../@local/hash-isomorphic-utils/src/format-number.ts 0.00% 1 Missing ⚠️
libs/@local/hash-isomorphic-utils/src/numbers.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6598      +/-   ##
==========================================
- Coverage   20.93%   20.85%   -0.08%     
==========================================
  Files         583      558      -25     
  Lines       20111    19418     -693     
  Branches     3002     2954      -48     
==========================================
- Hits         4210     4050     -160     
+ Misses      15844    15311     -533     
  Partials       57       57              
Flag Coverage Δ
apps.hash-ai-worker-ts 1.32% <ø> (+0.02%) ⬆️
apps.hash-api 0.00% <0.00%> (-1.14%) ⬇️
local.hash-backend-utils 4.04% <ø> (-4.78%) ⬇️
local.hash-graph-sdk 0.00% <ø> (-58.63%) ⬇️
local.hash-isomorphic-utils 0.00% <0.00%> (-0.90%) ⬇️
local.hash-subgraph 23.50% <ø> (-1.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +83 to +91
if (token === "self") {
value = sourceTitle;
} else if (typeof token === "string") {
value = operatorToOpCharacterMap[token];
} else if (Array.isArray(token)) {
throw new Error("Nested conversion expressions are not supported");
} else {
value = formatNumber(token.const);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue in your code:
String comparisons using '===', '!==', '!=' and '==' is vulnerable to timing attacks. A timing attack allows the attacker to learn potentially sensitive information by, for example, measuring how long it takes for the application to respond to a request. More info: https://nodejs.org/en/learn/getting-started/security-best-practices#information-exposure-through-timing-attacks-cwe-208

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by node_timing_attack.

You can view more details about this finding in the Semgrep AppSec Platform.

vilkinsons
vilkinsons previously approved these changes Mar 3, 2025
@CiaranMn CiaranMn added this pull request to the merge queue Mar 4, 2025
Merged via the queue into main with commit 945bed3 Mar 4, 2025
62 checks passed
@CiaranMn CiaranMn deleted the cm/data-conversions-and-entity-editor-fixes branch March 4, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/apps area/libs Relates to first-party libraries/crates/packages (area) type/eng > backend Owned by the @backend team type/eng > frontend Owned by the @frontend team
Development

Successfully merging this pull request may close these issues.

2 participants